Repository navigation
Add emoji reactions (limited set on notes) - #76767
adamsilverstein wants to merge 213 commits into
Conversation
|
Size Change: +2.71 kB (+0.03%) Total Size: 7.92 MB 📦 View Changed
|
|
Flaky tests detected in 4b59106. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/33358611256 As a user I expect my navigation to use the colors I selected for it in
|
|
Regarding unbounded requests & embedding reactions: If embedding all the reactions on a given note is too much, we could think about caching/retrieving only the reaction count (grouped by emoji). This way we can quickly display the different reactions on a note and lazy load the individual reactions (and who gave them) after that. |
|
Two open threads carried over from #75549 worth noting:
All other feedback from #75549 has been addressed — see my comparison for details. |
|
@swissspidy this is what's shipping now - in case you want to double-check it matches what you had in mind:
The unbounded |
|
I'm going to look at leveraging an existing library for the emoji picker itself, something like https://github.com/liveblocks/frimousse that is well maintained, license compatible and designed to be styled/extended. This would provide a much richer experience, and we can still start with the simplified design of a small set of emoji with the full selection "behind" a (+) plus button. |
|
Here is a screencast of the current implementation in action: react.with.emoji.picker.mp4 |
The excluded comment types are string literals, so they can go into the SQL directly instead of through placeholders. Type the hook callbacks to what core passes them, which also lets the WP_Comment guards go. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0126YH7wfjdvbPpuuNv1v92h
Co-authored-by: Weston Ruter <westonruter@gmail.com>
|
@westonruter I've resolved the comments regarding the existing core code. It might be best to focus the review only on the parts actually being changed in this PR. |
Unfortunately, the way we have to override the controller makes that process a bit harder. |
| // START CHANGE: Reject a reaction on behalf of another user. | ||
| /* | ||
| * A reaction is always first-person: the uniqueness check and the | ||
| * reaction summary key on the current user, so one stored against | ||
| * somebody else could never be counted or removed. | ||
| */ | ||
| if ( | ||
| 'reaction' === $request['type'] && | ||
| isset( $request['author'] ) && get_current_user_id() !== (int) $request['author'] | ||
| ) { | ||
| return new WP_Error( | ||
| 'rest_comment_invalid_author', | ||
| __( 'Sorry, you are not allowed to add a reaction on behalf of another user.', 'gutenberg' ), | ||
| array( 'status' => rest_authorization_required_code() ) | ||
| ); | ||
| } | ||
| // END CHANGE. |
There was a problem hiding this comment.
Users with the moderate_comments capability can create comments and notes on behalf of other users, so at first I wasn't sure we needed a special check only for reactions. That said, a user can react with the same emoji only once per note, and the uniqueness check keys on the current user, so this check might make sense.
@Mamaduka @westonruter What do you think?
| function gutenberg_filter_comment_count_query_exclude_block_comments_7_2( $query ) { | ||
| if ( str_starts_with( $query, 'SELECT comment_post_ID, COUNT(comment_ID) as num_comments FROM' ) && str_contains( $query, 'comment_approved' ) ) { | ||
| // Add an exclusion clause for each internal type not already present. | ||
| // Core's query already hardcodes the note-only exclusion, so expanding | ||
| // per type - rather than bailing when any exclusion exists - ensures | ||
| // reactions are excluded too and keeps the filter idempotent if it | ||
| // runs more than once. | ||
| $type_clauses = array(); | ||
| foreach ( array( 'note', 'reaction' ) as $internal_type ) { | ||
| $clause = "comment_type != '" . esc_sql( $internal_type ) . "'"; | ||
| if ( ! str_contains( $query, $clause ) ) { | ||
| $type_clauses[] = $clause; | ||
| } | ||
| } | ||
| if ( ! empty( $type_clauses ) ) { | ||
| $query = str_replace( 'comment_approved', implode( ' AND ', $type_clauses ) . ' AND comment_approved', $query ); | ||
| } | ||
| } | ||
| return $query; | ||
| } |
There was a problem hiding this comment.
Since reactions are always created as approved and can't be updated, they should never be counted in this pending comments query (comment_approved = '0'). Could we just remove this filter?
|
The editor code tests well. It looks like only the remaining items are for the REST API controller. Should we aim to land this before the end of this week, so the feature is ready for the next Gutenberg RC on Monday? |
|
@Mamaduka, I think this PR is basically ready to go, but would you mind if I pushed some changes to address the server-side feedback left by @westonruter and me? |
|
@t-hamano, please do that; it would be very helpful. Thank you! |
… input Co-Authored-By: Claude <noreply@anthropic.com>
…d create input" This reverts commit 85f63c1. Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Match core, which defines get_note_reaction_keys() as a private static method on WP_REST_Comments_Controller. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
…om core Co-Authored-By: Claude <noreply@anthropic.com>
|
I think most of the feedback has been addressed. Here's a summary of the commits I pushed:
Here's a summary of the remaining open comments:
Finally, note that these changes haven't been backported to the core PR (#10930) yet. If I have time tomorrow, I plan to backport them to core. I think this is ready for a final round of feedback! |
Mamaduka
left a comment
There was a problem hiding this comment.
Thank you, @t-hamano!
Going to pre-approve this. We often sync changes back from core backport PRs, and I think it's much easier to review PHP code (fewer lines, etc.).
@westonruter, what do you think?
|
Thanks @t-hamano for pushing all the updates here, appreciate the extra effort here to get this over the finish line before Beta 1. 🎉
I will look at this again, but don't think it should be a blocker to merging. The core side will be a little cleaner, but still messy because of the lack of a formal API. |
|
@adamsilverstein, I have just finished syncing the updated PHP code from this PR into the core PR. I believe both PRs are now ready to go. As long as the server-side implementation around comment permissions and security is robust, we should be able to address the client-side logic and UI after Beta 1. |
We can address most paper cuts after in beta cycle; features usually get more testing once they're merged. |
Excellent! I'll give it a review pass. |
Summary
Closes #75144
Adds the foundation for emoji reactions on block notes. This PR is scoped to a small curated 5-emoji quick row (heart, celebration, smile, eyes, rocket). The full searchable emoji picker behind a
+button lives in a follow-up PR stacked on this one - per @t-hamano's request, so library/bundling decisions can be reviewed separately.Storage: reactions as a custom comment type
Reactions are stored as individual
reactioncomment records (one per user-emoji-note triple), as suggested by @swissspidy, in preference to JSON blobs in comment meta:This is more flexible, aligns with WordPress data patterns (similar to the React plugin), and could eventually extend to regular comments. The custom type is hidden from
wp-admin/edit-comments.phpand excluded from comment counts.Curated emoji set
The default 5 emojis (heart, celebration, smile, eyes, rocket) are stored as ASCII slugs (e.g.
heart). Storing slugs sidesteps utf8 vs utf8mb4 portability issues on the comments table and gives stable grouping in thereaction_summaryaggregation.The list is filterable via the
gutenberg_note_reaction_emojisPHP filter and exposed to clients through the REST API schema (reaction_emojisproperty onOPTIONS /wp/v2/comments), implementing the approach suggested by @Mamaduka.Efficient reaction loading
Each note response includes a
reaction_summaryfield -{ [emojiSlug]: { count, reacted, my_reaction_id } }. The schema lives inclass-gutenberg-rest-comment-controller-7-1.php, populated inprepare_item_for_response().get_items()pre-fetches summaries for the entire result set with one aggregatedGROUP BYquery plus one query for the current user's own reactions, regardless of how many notes are returned (seeprefetch_reaction_summaries()).reaction_summary- names are fetched on tooltip hover, so the initial notes load stays cheap.Popover architecture
The curated picker uses
@wordpress/components/Dropdown, whose Popover portals to<body>. This is essential because the collab sidebar has anoverflow: hiddenchain reaching all the way up to.interface-interface-skeleton__sidebar(framework-level); a non-portaled popover would clip. The note thread'sonBlurhandler exempts.components-popoverso that focus moving into the picker doesn't deselect the note (which would unmount the trigger and close the picker mid-click).Where the trigger lives
The 😊 trigger floats in the note's top corner, in the same column as Resolve and the actions menu, and stays faded until the note is hovered or the trigger itself is focused. A note nobody has reacted to costs no vertical space for the option, and the note text keeps a safe area on that side so it never runs underneath. Once a reaction lands the row drops into the flow, and the trigger trails the pills and wraps with them. This follows @annezazu's suggestion and @fcoveram's exploration, and was first built in #82313 on the follow-up PR before moving here per @draganescu's question.
tabIndex={ 0 }, so a thread-level:focus-withinwould hold for as long as the note is selected, and a thread-level:hoverwould reveal the trigger on every note in the thread at once.@media (hover: hover), so a touch device keeps a trigger it can actually see.Buttonvariants, no custom CSS.Related
Remaining feedback from #75549
useEntityRecordsfor reactions has noper_pagelimit. Consider lazy-loading reactions only when a note thread is opened. Comment@wordpress/icons(@ellatrix) - Low priority, fine for now. CommentAlready addressed
wordpress-7.1compat dir (@Mamaduka, @t-hamano) - Donereaction(@swissspidy) - Donegutenberg_note_reaction_emojisfilter, surfaced via REST schemaScreenshots
Screencast
Hover to reveal the trigger, react twice, then deselect the note:
Testing
For testing with multiple users, use the User Switching Plugin - https://wordpress.org/plugins/user-switching/
npm run test:e2e -- test/e2e/specs/editor/various/block-notes.spec.jsnpm run test:unit:php:base -- --filter=Gutenberg_REST_CommentsAI Use
Code and description both written with 🤖 Claude Code, with plenty of steering. I will review and test.